The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Sep 19, 2025 0:48am

Claude finished @NathanFlurry's task —— View job

PR Review: SSE Connection/Disconnection Test Fixes

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changes in packages/cloudflare-workers/vitest.config.ts
• Analyze changes in packages/rivetkit/src/driver-test-suite/tests/actor-conn-state.ts
• Provide comprehensive review feedback on code quality, bugs, performance, security, and test coverage

Summary of Changes

This PR fixes connection/disconnection tests for Server-Sent Events (SSE) by addressing timing issues and test logic problems in Cloudflare Workers environment.

File 1: packages/cloudflare-workers/vitest.config.ts

• Increased test timeout from 15 seconds to 60 seconds to accommodate slower package installation

File 2: packages/rivetkit/src/driver-test-suite/tests/actor-conn-state.ts

• Fixed test logic in the connection lifecycle test
• Added proper connection tracking assertions
• Improved timing for SSE disconnection detection

Code Quality Assessment

✅ Positive Aspects:

1. Proper timeout handling: The vitest config change appropriately addresses the slower execution environment of Cloudflare Workers
2. Improved test reliability: The test fixes address race conditions specific to SSE implementations
3. Clear comments: Good use of comments explaining SSE-specific timing issues (// SSE takes a long time to disconnect on CF Workers)

⚠️ Issues and Concerns:

1. Critical Bug - Variable Reference Error (packages/rivetkit/src/driver-test-suite/tests/actor-conn-state.ts:176)

   const disconnects = await newConn.getDisconnectionCount();

   This references newConn before it's declared (line 187). This will cause a ReferenceError at runtime.

2. Test Logic Issue (packages/rivetkit/src/driver-test-suite/tests/actor-conn-state.ts:192)

   const connectionIds = await conn.getConnectionIds(); // Should be newConn

   Using disposed connection conn instead of active newConn connection.

3. Inconsistent Test Assertions (packages/rivetkit/src/driver-test-suite/tests/actor-conn-state.ts:164-168)

   const disconnects = await conn.getDisconnectionCount();
   expect(disconnects).toBe(0);

   This assumes initial state is 0, but the test is checking disconnection count on a connection that may have previous state.

4. Race Condition Risk: The test still has potential race conditions between connection disposal and disconnection count updates, even with the increased timeout.

Performance Considerations

• Timeout Increase: The 4x timeout increase (15s → 60s) seems excessive. Consider if a more modest increase (e.g., 30s) would suffice.
• Test Efficiency: Multiple vi.waitFor calls with 10-second timeouts may make tests slower than necessary.

Security Concerns

No security issues identified. The changes are limited to test configuration and test logic.

Test Coverage

Improvements Needed:

1. Fix the variable reference errors that will cause test failures
2. Consider testing edge cases for SSE disconnection timing
3. Add assertions to verify the exact sequence of events in connection lifecycle

Recommendations

Critical Fixes Required:

// Fix 1: Move newConn declaration before first usage
const newConn = handle.connect();

// Verify disconnection was tracked
await vi.waitFor(
    async () => {
        const disconnects = await newConn.getDisconnectionCount();
        expect(disconnects).toBe(1);
    },
    { timeout: 10_000, interval: 100 }
);

// Fix 2: Use correct connection reference
await vi.waitFor(async () => {
    const connectionIds = await newConn.getConnectionIds(); // Changed from conn
    expect(connectionIds.length).toBe(2);
});

General Improvements:

1. Consider reducing the vitest timeout to 30s instead of 60s
2. Add more specific error messages to waitFor assertions
3. Consider extracting timeout constants for better maintainability

The core approach of fixing SSE timing issues is sound, but the implementation has critical bugs that need to be addressed before merging.

• chore(core): migrate gateway to use websocket protocols #1293
• chore(core): update api schema to match new engine schema #1292
• chore(core): fix conn/disconn tests for sse #1290 👈 (View in Graphite)
• fix(core): fix inline client driver for cloudflare-workers #1289
• chore(core): fix missing transport for inline tests #1287 : 1 other dependent PR (#1250 )
• chore(core): fix test for sending message to specific connection sse race condition #1286
• fix(cloudflare-workers): fix alarms #1285
• chore(core): fix broadcast race condition #1284
• chore: list sideEffects #1283
• chore(core): fix connection tests to support sse #1282
• fix(cloudflare-workers): fix aborts on sse streams on cloudflare workers #1281
• fix(cloudflare-workers): fix aborting sse streams #1280
• chore: remove unused FakeEventSource #1279
• main

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• merge queue - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

This stack of pull requests is managed by Graphite. Learn more about stacking.

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/actor@1290

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/cloudflare-workers@1290

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/core@1290

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/db@1290

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/framework-base@1290

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/next-js@1290

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/react@1290

pnpm add https://pkg.pr.new/rivet-dev/rivetkit@1290

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/sql-loader@1290

commit: 9ea5720

Merge activity

• Sep 24, 6:06 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Sep 24, 6:07 AM UTC: CI is running for this pull request on a draft pull request (#1294) due to your merge queue CI optimization settings.
• Sep 24, 6:08 AM UTC: Merged by the Graphite merge queue via draft PR: #1294.